Proof-of-concept: Rewrite to ts #212
Conversation
Co-authored-by: Patrik Åkerstrand <[email protected]>
There was a problem hiding this comment.
First of all I would like you thank you for this contribution, it is great progress in terms of TS integration 🙇
I had a few rather small comments regarding structuring of the project etc. but other than that it looks really good and reasonable 👌
We can discuss on priv how to move forward with that.
| #!/bin/bash | ||
|
|
||
| rm -rf es5 |
There was a problem hiding this comment.
I think this file is not used 🤔
There was a problem hiding this comment.
It's the folder webpack use to output es5 into, so at least for now it is still used and should be cleaned I think. (This was the only non-webpack command from the old build.sh)
| "prebuild": "npm run clean", | ||
| "build": "npm run build:types && npm run build:js", | ||
| "build:types": "tsc --emitDeclarationOnly", | ||
| "build:js": "webpack" |
There was a problem hiding this comment.
By default it could build only prod and prodGlobal configs (e.g. could be done by specifying config name and using config-name flag)
There was a problem hiding this comment.
True, I was mainly concerned with migrating existing functionality 1:1, but lets note that as an improvement. With the new setup webpack runs all the tasks in parallell tho, so it won't make that much of a difference.
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
Probably it would make sense to specify outDir to the same one as in webpack config as ideally we would release only dist directory to reduce bundle size.
There was a problem hiding this comment.
Once everything is converted to TS then that would make sense, but for this partial rewrite to work we cannot do that just yet.
| @@ -3,3 +3,4 @@ | |||
| coverage | |||
There was a problem hiding this comment.
Note to myself: we need to add eslint typescript plugin
| @@ -2,175 +2,126 @@ | |||
| * See LICENSE.md in the project root. | |||
There was a problem hiding this comment.
Just 2 general thoughts regarding this file:
- small thing, you've changed strings from this notation '' to "" 😅
- in some cases (e.g.
IdentityOptions) you've introduced duplicate declarations, both inidentity.d.tsandidentity.tsfile - is there any reason for that?
There was a problem hiding this comment.
Sorry about the first, I guess lack of ts-lint and/or prettier config made " my formatting default. Will ofc fix this!
Regarding duplicates in identity.d.ts & identity.ts those are expected since identity.d.ts is now generated from identity.ts then all exported members should be present in both files. (Unless I misunderstood something).
|
@filipprasalek thank you for reviewing this! |
Proof of concept for rewriting SDK to TS.
This PR contains the following changes:
What needs to be done?
The following has not been done and should be fixed after the above has been decided:
Co-authored-by: @PAkerstrand